Add generic trait to combine UnixListener and TcpListener #4385

cecton · 2022-01-07T13:50:14Z

Motivation

I would like to have a generic type for creating a socket from TCP or from a Unix socket so I don't need to duplicate code.

Solution

~~Added a new type that combines the SocketAddr of TcpListener and the one of UnixListener. I called it ListenAddr.~~ [edit: opted for associated type]
Added a new trait Listener with an accept() method and a local_addr() method to reflect the methods from TcpListener and UnixListener. [edit: also accept_poll()]
~~Added a new trait AsyncReadWrite which combines AsyncRead and AsyncWrite. This is because I will need to box the stream.~~ [edit: opted for associated type]
~~Added a method set_nodelay() to reflect the method set_nodelay() of TcpStream which I needed.~~ [edit: removed because it can be handled by the user easily]

Problem encountered

There is a module "udp" at root of tokio-util. It should probably moved to the new module "net" I created. One alternative would be to rename the "unix" module I created to something else.

Follow up of #4244

Darksonn · 2022-01-07T15:34:10Z

tokio-util/src/net/unix/mod.rs

+#[derive(Debug)]
+pub enum ListenAddr {
+    /// Socket address.
+    SocketAddr(std::net::SocketAddr),
+    /// Unix socket.
+    UnixSocket(tokio::net::unix::SocketAddr),
+}


This should probably be an associated type of the Listener trait like how Io is set up.

Done in 9bda024

Darksonn · 2022-01-07T15:39:50Z

tokio-util/src/net/unix/mod.rs

+/// A trait for a listener: `TcpListener` and `UnixListener`.
+pub trait Listener: Send + Unpin {
+    /// The stream's type of this listener.
+    type Io: tokio::io::AsyncRead + tokio::io::AsyncWrite;
+
+    /// Accepts a new incoming connection from this listener.
+    fn accept<'a>(
+        &'a self,
+    ) -> Pin<Box<dyn Future<Output = Result<(Self::Io, ListenAddr)>> + Send + 'a>>;
+
+    /// Returns the local address that this listener is bound to.
+    fn local_addr(&self) -> Result<ListenAddr>;
+}


I think the trait should look more like this:

Suggested change

/// A trait for a listener: `TcpListener` and `UnixListener`.

pub trait Listener: Send + Unpin {

/// The stream's type of this listener.

type Io: tokio::io::AsyncRead + tokio::io::AsyncWrite;

/// Accepts a new incoming connection from this listener.

fn accept<'a>(

&'a self,

) -> Pin<Box<dyn Future<Output = Result<(Self::Io, ListenAddr)>> + Send + 'a>>;

/// Returns the local address that this listener is bound to.

fn local_addr(&self) -> Result<ListenAddr>;

}

/// A trait for a listener: `TcpListener` and `UnixListener`.

pub trait Listener {

/// The stream's type of this listener.

type Io: tokio::io::AsyncRead + tokio::io::AsyncWrite;

/// The address of the new connection.

type Addr;

fn poll_accept(self: Pin<&mut self>) -> Poll<(Self::Io, Self::ListenAddr)>;

/// Returns the local address that this listener is bound to.

fn local_addr(&self) -> Result<ListenAddr>;

/// Accepts a new incoming connection from this listener.

fn accept<'a>(&'a mut self) -> ListenerAcceptFut<'a, Self>

where

Self: Sized + Unpin,

{

ListenerAcceptFut::new(self)

}

}

Then, the crate would define a ListenAcceptFut type that implements Future with a manual impl similar to the return types in the Ext traits.

Done in c2ec467

cecton · 2022-01-12T13:37:01Z

I'm trying out with the current API using type alias but it seems it doesn't work with my use case because the TCP address/Unix socket path is provided at runtime so I would need to dynamic dispatch somewhere. Box<dyn Listener> doesn't work because the type aliases are required.

Darksonn · 2022-01-12T14:04:08Z

You can implement the trait for the Either type also in tokio-util to get what you want.

cecton · 2022-01-12T14:39:52Z

Oh I think I see! Thanks!

cecton · 2022-01-13T11:47:49Z

@Darksonn ok I think I'm ready for review

cecton · 2022-01-13T16:26:17Z

(Fixed clippy stuff)

tokio-util/src/net/mod.rs

Darksonn · 2022-01-13T19:47:39Z

tokio-util/src/net/mod.rs

+    /// Polls to accept a new incoming connection to this listener.
+    fn poll_accept(&self, cx: &mut Context<'_>) -> Poll<Result<(Self::Io, Self::Addr)>>;
+
+    /// Accepts a new incoming connection from this listener.
+    fn accept(&self) -> ListenerAcceptFut<'_, Self>


I think it's best to accept a &mut self here. Calling accept twice in parallel will result in one of them breaking due to lost wakeups.

Suggested change

/// Polls to accept a new incoming connection to this listener.

fn poll_accept(&self, cx: &mut Context<'_>) -> Poll<Result<(Self::Io, Self::Addr)>>;

/// Accepts a new incoming connection from this listener.

fn accept(&self) -> ListenerAcceptFut<'_, Self>

/// Polls to accept a new incoming connection to this listener.

fn poll_accept(&mut self, cx: &mut Context<'_>) -> Poll<Result<(Self::Io, Self::Addr)>>;

/// Accepts a new incoming connection from this listener.

fn accept(&mut self) -> ListenerAcceptFut<'_, Self>

That would make the API different from tokio's TCP listener and Unix listener. If they don't use &mut self there, why would I need it here?

The accept fn in Tokio will not have the same lost wakeup issue as this one does because it isn't implemented by calling poll_accept on itself. However, we cant really replicate what Tokio does with a trait. The &mut self avoids the footgun.

Ok I see!! Thanks for the explanation

Fixed in fe231ec

Darksonn · 2022-01-13T19:48:01Z

tokio-util/src/net/mod.rs

+    /// Polls to accept a new incoming connection to this listener.
+    fn poll_accept(&self, cx: &mut Context<'_>) -> Poll<Result<(Self::Io, Self::Addr)>>;


This should take a pinned reference to self.

I replicated the signature of the function from tokio's TCP listener. Are you sure you want to make it different?

In general, making this take &self will make it difficult to implement this trait for anything else than the two listener types in Tokio itself. This is why I suggested changing it.

Fixed in fe231ec

Darksonn · 2022-01-13T19:48:40Z

tokio-util/src/net/unix/mod.rs

+use super::Listener;
+use std::io::Result;
+use std::task::{Context, Poll};
+
+impl Listener for tokio::net::UnixListener {
+    type Io = tokio::net::UnixStream;
+    type Addr = tokio::net::unix::SocketAddr;
+
+    fn poll_accept(&self, cx: &mut Context<'_>) -> Poll<Result<(Self::Io, Self::Addr)>> {
+        Self::poll_accept(self, cx)
+    }
+
+    fn local_addr(&self) -> Result<Self::Addr> {
+        self.local_addr().map(Into::into)
+    }
+}


Just put this in the other folder.

This will force me to add #[cfg(unix)].

Ok, I don't mind whether it is separate or together then.

Ok I'll keep it then but I don't mind either way either

Co-authored-by: Alice Ryhl <[email protected]>

cecton · 2022-01-14T11:17:21Z

I think this is ready for review again.

Darksonn · 2022-01-14T14:47:56Z

Your branch is 59 commits behind tokio-rs:master and is therefore missing a fix for the CI failure in the two FreeBSD CI runs. Please merge master into your branch.

Darksonn · 2022-01-14T14:50:10Z

tokio-util/src/net/mod.rs

+    /// Polls to accept a new incoming connection to this listener.
+    fn poll_accept(&mut self, cx: &mut Context<'_>) -> Poll<Result<(Self::Io, Self::Addr)>>;
+
+    /// Accepts a new incoming connection from this listener.
+    fn accept(&mut self) -> ListenerAcceptFut<'_, Self>
+    where
+        Self: Sized + Unpin,
+    {


Either poll_accept should take self: Pin<&mut Self>, or the Unpin bound should be removed from accept.

Fixed in a538f0b

cecton · 2022-01-14T17:50:32Z

Your branch is 59 commits behind tokio-rs:master and is therefore missing a fix for the CI failure in the two FreeBSD CI runs. Please merge master into your branch.

oops 😅 I didn't notice somehow, sorry! Fixed in 812dfa5

And ready for review.

tokio-util/src/net/mod.rs

Darksonn · 2022-01-24T13:51:57Z

tokio-util/src/net/mod.rs

+    fn poll_accept(&mut self, cx: &mut Context<'_>) -> Poll<Result<(Self::Io, Self::Addr)>> {
+        match self {
+            Either::Left(listener) => listener
+                .poll_accept(cx)
+                .map(|res| res.map(|(stream, addr)| (Either::Left(stream), Either::Left(addr)))),
+            Either::Right(listener) => listener
+                .poll_accept(cx)
+                .map(|res| res.map(|(stream, addr)| (Either::Right(stream), Either::Right(addr)))),
+        }
+    }


Hmm. It's unfortunate that the types here allow it return stuff like (Left(io), Right(addr)) even though it wont in practice. Ideas?

Wow I'm so blind I didn't even notice.

Yes you're right in practice this will never happen. If someones use match on the Result, they will need to make unreachable branches just for that. It's stupid.

.... 🤔 I will think about it... but I don't see a solution right now.

Probably making yet another Either specialized for Listener could solve the issue.

I noticed the Either type is something not universal anyway: tokio-util has one but tower also has its own: https://docs.rs/tower/latest/tower/util/enum.Either.html

I assume we could have a "EitherListener" with a poll_accept method that would return Result<Either<(IoLeft, AddrLeft), (IoRight, AddrRight)>>. The enum itself would look like this I guess:

pub enum EitherListener<L: Listener, R: Listener> { Left(L), Right(R), }

I don't think the user absolutely "needs" to use tokio-util's Either, it could definitely be something more specialized. wdyt?

I'm not convinced that adding an EitherListener enum changes anything here?

EitherListener would not implement Listener, it would have its own method with their own signatures

Another option is to change the trait so that Addr and Io are merged into a single type which each listener will define as a pair.

That would work too but not super elegant I guess...

You could just put the method on the Either we already have.

Good point

Ok I replaced the Listener impl on Either by custom methods here: b55b62c3

Unfortunately the accept Future struct is slightly different so I had to duplicate some code.

Why not just make it an async fn and drop the poll function? We don't need these tricks when it's not a trait method.

Done in 316f015

Co-authored-by: Alice Ryhl <[email protected]>

tokio-util/src/net/mod.rs

cecton · 2022-01-27T16:49:08Z

Thank you for your time

Darksonn · 2022-01-27T21:44:06Z

Thank you for the PR!

cecton added 3 commits November 18, 2021 10:20

Add generic trait to combine UnixListener and TcpListener

4305804

Remove unnecessary move

69b0379

Replace Box by associated type and remove unused trait

cd1ddf9

Darksonn added the A-tokio-util Area: The tokio-util crate label Jan 7, 2022

Darksonn reviewed Jan 7, 2022

View reviewed changes

Darksonn mentioned this pull request Jan 10, 2022

Add an Either type for Listeners #4048

Closed

Remove ListenAddr enum & replace with type alias

9bda024

cecton added 3 commits January 12, 2022 15:54

Impl Listener for Either

d0dba7a

Add poll_accept & struct that impl Future

c2ec467

Move things to the right place

8afdc3e

cecton marked this pull request as ready for review January 13, 2022 11:47

cecton mentioned this pull request Jan 13, 2022

Add unix socket apollographql/router#158

Merged

Clippy fixes

75dcc76

Darksonn reviewed Jan 13, 2022

View reviewed changes

cecton and others added 2 commits January 14, 2022 12:11

Update tokio-util/src/net/mod.rs

4a46da1

Co-authored-by: Alice Ryhl <[email protected]>

Use &mut self instead of &self for accept() and poll_accept()

fe231ec

Darksonn reviewed Jan 14, 2022

View reviewed changes

cecton added 2 commits January 14, 2022 18:46

Merge commit 4eed411 (no conflict)

812dfa5

Remove Unpin from accept()

a538f0b

Darksonn reviewed Jan 24, 2022

View reviewed changes

cecton and others added 2 commits January 24, 2022 15:22

Update tokio-util/src/net/mod.rs

e194407

Co-authored-by: Alice Ryhl <[email protected]>

Replace Listener impl on Either with custom methods

b639d98

cecton added 2 commits January 25, 2022 10:23

Remove unnecessary new() functions

27912b7

Remove EitherListenerAcceptFut

316f015

Darksonn reviewed Jan 25, 2022

View reviewed changes

tokio-util/src/net/mod.rs Outdated Show resolved Hide resolved

Removed Sized

61bd0fa

Darksonn approved these changes Jan 27, 2022

View reviewed changes

Darksonn merged commit 8f77ee8 into tokio-rs:master Jan 27, 2022

cecton deleted the cecton-unix-tcp-listener branch January 27, 2022 16:48

tobz mentioned this pull request Feb 10, 2022

chore: prepare tokio-util 0.7.0 #4486

Merged

		/// Polls to accept a new incoming connection to this listener.
		fn poll_accept(&self, cx: &mut Context<'_>) -> Poll<Result<(Self::Io, Self::Addr)>>;

Add generic trait to combine UnixListener and TcpListener #4385

Add generic trait to combine UnixListener and TcpListener #4385

Conversation

cecton commented Jan 7, 2022 • edited Loading

Motivation

Solution

Problem encountered

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Jan 12, 2022

Darksonn commented Jan 12, 2022

cecton commented Jan 12, 2022

cecton commented Jan 13, 2022

cecton commented Jan 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darksonn Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Jan 14, 2022

Darksonn commented Jan 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Jan 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Jan 27, 2022

Darksonn commented Jan 27, 2022

cecton commented Jan 7, 2022 •

edited

Loading

Darksonn Jan 13, 2022 •

edited

Loading